Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Implement knownForType for RFC#58. #95

Merged
merged 2 commits into from
Jun 12, 2015

Conversation

rwjblue
Copy link
Member

@rwjblue rwjblue commented Jun 11, 2015

Supports emberjs/ember.js#11393.

Further reading: emberjs/rfcs#58.

@rwjblue
Copy link
Member Author

rwjblue commented Jun 11, 2015

I split this into two commits to make it easier to review. The first commit is the actual change, and the second is updating dist/.

eachForType: function(type, callback) {
var moduleEntries = requirejs.entries;

for (var moduleName in moduleEntries) {
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Ember.keys

@stefanpenner
Copy link
Contributor

i believe in the past the idea was Array.isArray(resolveAllForType(name)) === true, any reason why we instead opted for the current iterator style?

The array form is more flexible, especially in light of the fun iterables have in es6+

@rwjblue
Copy link
Member Author

rwjblue commented Jun 11, 2015

@stefanpenner - I don't fully understand your suggestion. Can you elaborate? What would 'resolveAllForType' return (or do)?

@rwjblue
Copy link
Member Author

rwjblue commented Jun 11, 2015

The current primary use-case for this is to enumerate the list of names for the current type for the dashless helper RFC. In that case we actually do not have a use for the factories (since we are just whitelisting the helper names themselves), but I added them here because I think we will find that useful in the future.

I could definitely see this method returning an object instead of calling a callback if you would prefer that, but I don't see how an array would work (since we need both a key and a value).

@stefanpenner
Copy link
Contributor

@stefanpenner - I don't fully understand your suggestion. Can you elaborate? What would 'resolveAllForType' return (or do)?

  • So this feature looks useful for more then just dashless helpers, but also initializers and instance initializers.
  • Also, rather then locking us into having to use forEach to enumerate, by returning an array (instead of just exposing forEach), We can also filter/map/inject/for in/for on the collection. Basically it just feels more flexible.

@rwjblue
Copy link
Member Author

rwjblue commented Jun 11, 2015

@stefanpenner - To confirm, you would prefer something like the following:

var items = resolver.eachForType('fruit');

/*
  Where items is something like:
  {
    'fruit:apple': AppleFactory,
    'fruit:orange': OrangeFactory
  }
*/

Right?

@teddyzeenny
Copy link
Contributor

Looks similar to what container-debug-adapter does. Should we consider removing the container debug adapter?

@rwjblue
Copy link
Member Author

rwjblue commented Jun 11, 2015

@teddyzeenny - I think that could be refactored to use the new method here, but I'm not sure that it completely replaces the container debug adapter.

@stefanpenner
Copy link
Contributor

Right?

i was actually just thinking an array of names, but an array of well formed pojos is also really cool

@rwjblue rwjblue force-pushed the add-each-for-type branch from 3c915c3 to 4001081 Compare June 11, 2015 19:00
@rwjblue
Copy link
Member Author

rwjblue commented Jun 11, 2015

@stefanpenner - Updated to address your concerns (as I understood them):

  • Swapped to using Ember.keys instead of for/in.
  • Remove callback argument so a plain dictionary is returned.

Can you double check, and 👍 / 👎?

@stefanpenner
Copy link
Contributor

Remove callback argument so a plain dictionary is returned.

as mentioned above, an array of well formed pojos seems like the must flexible return value.

@rwjblue
Copy link
Member Author

rwjblue commented Jun 11, 2015

@stefanpenner - Not sure that I agree.

How is an array of key/value objects better than an actual structure that is built to deal with keys and values (a single object)?

[
  {key: 'fruit:apple', value: AppleFactory},
  {key: 'fruit:orange', value: OrangeFactory}
]

vs

{
  'fruit:apple': AppleFactory,
  'fruit:orange': OrangeFactory
}

Are you concerned with needing to add more things than key (container fullname) and value (actual factory)?

@rwjblue
Copy link
Member Author

rwjblue commented Jun 11, 2015

In the end I just want to settle this so we can move forward on the dashless helpers RFC which needs to be out in 1.13 tomorrow.

If you are certain that you want an array of objects, I will change to that API.

@stefanpenner
Copy link
Contributor

How is an array of key/value objects better than an actual structure that is built to deal with keys and values (a single object)?

the common pattern is to enumerate/filter/map/transform this information. It seems like exposing an iterable primitive is appropriate.

@stefanpenner
Copy link
Contributor

helpers RFC which needs to be out in 1.13 tomorrow.

this is true, but once we land this it will be around for ages, so being mindful and diligent now saves us from prolonged pain

@rwjblue
Copy link
Member Author

rwjblue commented Jun 11, 2015

this is true, but once we land this it will be around for ages, so being mindful and diligent now saves us from prolonged pain

Agreed, but please remember that this is also a private API used only by Ember itself ATM.

@rwjblue
Copy link
Member Author

rwjblue commented Jun 11, 2015

Updated to use an array of objects as requested.

@stefanpenner
Copy link
Contributor

hmm, your likely going to be frustrated by this, but I just realized these iterables may yield duplicates and we have lost the precedence provided during resolving.

for example:

app/components/edit-user.js
app/pods/edit-user/componeont.js

clearly in resolving, we realize the later shadows the former.

@rwjblue
Copy link
Member Author

rwjblue commented Jun 11, 2015

Yes, using the object form automatically de-dupes for us and allows us to ensure the proper ordering.

If your primary concern with the object case is future expansion in what the values should be (for example maybe we also want modulePath), I could make something like:

{
  'fruit:apple': {key: 'fruit:apple', value: AppleFactory},
  'fruit:orange': {key: 'fruit:orange', value: OrangeFactory}
}

That still uses an object for de-duping and prioritization, but allows expansion on the number of keys in the object later...

@stefanpenner
Copy link
Contributor

@rwjblue the object form clearly dedupes, but I am unclear where in the code it takes the priority into account

@rwjblue
Copy link
Member Author

rwjblue commented Jun 11, 2015

This PR only addresses the resolver parts, not registry or fallback registry. That is done in the Ember PR which will have the order (later entries overriding earlier ones) as fallback registry, registry, resolver.

I do not currently handle ordering of different module types in this PR. So if you had both a pod style and non-pod style module with the same name whichever was last in requirejs.entries would win out. I could add a queueing system that works like moduleNameLookupPatterns here if you would like.

@stefanpenner
Copy link
Contributor

I could add a queueing system that works like moduleNameLookupPatterns here if you would like.

I would be willing to forgo this if you can tell me why its not important. It seems important?

@rwjblue
Copy link
Member Author

rwjblue commented Jun 11, 2015

It seems important?

It is not important for the current need. We currently only need to know the names of the types in the system, and we do absolutely nothing with the factory or other information returned. All we need to know is whether a specific name exists (nothing more).

My needs for RFC#58 would be satisfied if eachForType simply returned a POJO like:

{
  'helper:t': true,
  'helper:blah': true
}

We could go with a simple true value (it is the minimum requirement ATM), we can always "upgrade" it to an object if we need to, and any usage of it (like if (returnValueFromEachForType['helper:t']) { }) would still work if/when we need to flesh it out as an object with more values (since that would also be truthy)...

@stefanpenner
Copy link
Contributor

It is not important for the current need. We currently only need to know the names of the types in the system, and we do absolutely nothing with the factory or other information returned. All we need to know is whether a specific name exists (nothing more).

Ah this was the missing piece of information. Thank you! Priority/dedupe concern alleviated.

My needs for RFC#58 would be satisfied if eachForType simply returned a POJO like:

then this method name is likely misleading. As the result isn't an iterable rather an index/set of existence.

some ideas:

existsByType(type) === {
  'type:name: true
}

presentByType(type) === {
  'type:name;: true
}

@rwjblue rwjblue force-pushed the add-each-for-type branch from 0fad9f4 to 976ebcd Compare June 12, 2015 03:17
@rwjblue rwjblue changed the title Implement eachForType for RFC#58. Implement knownForType for RFC#58. Jun 12, 2015
@rwjblue
Copy link
Member Author

rwjblue commented Jun 12, 2015

Renamed to knownForType, and went with simple true value dictionary.

@stefanpenner
Copy link
Contributor

sg

@mixonic
Copy link
Member

mixonic commented Jun 12, 2015

lgtm :-D

rwjblue added a commit that referenced this pull request Jun 12, 2015
Implement `knownForType` for RFC#58.
@rwjblue rwjblue merged commit b801569 into ember-cli:master Jun 12, 2015
@rwjblue rwjblue deleted the add-each-for-type branch June 12, 2015 03:37
@stefanpenner
Copy link
Contributor

thanks for going on this journey with me

if (module && module['default']) { module = module['default']; }

if (module === undefined) {
if (defaultExport === undefined) {
throw new Error(" Expected to find: '" + parsedName.fullName + "' within '" + normalizedModuleName + "' but got 'undefined'. Did you forget to `export default` within '" + normalizedModuleName + "'?");
}

if (this.shouldWrapInClassFactory(module, parsedName)) {
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

should module be renamed to defaultExport here?

Copy link
Member Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Only if we would prefer to avoid an error 😜

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

4 participants